Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Re-bundle server app in adapter-node #1648

Merged
merged 5 commits into from
Jun 10, 2021
Merged

Re-bundle server app in adapter-node #1648

merged 5 commits into from
Jun 10, 2021

Conversation

Conduitry
Copy link
Member

Fixes #1257. This makes adapter-node run the server-side app through esbuild as part of the build process, treating anything in (prod) dependencies as external, and bundling everything else. This means that, for example, @sveltejs/kit will no longer need to be installed when deploying the production app - and any other dependencies that the app adds as devDependencies will also be bundled in. This means that, in production, you should be able to copy over your package.json and build/, run npm install --production, and run node build - which is what you'd expect to be able to do.

These changes seem to behave as expected on the project I have at work (which adds a number of dependencies, some of which should be bundled on the server and some of which should not) - but I'd still appreciate some more eyes on this PR.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

@benmccann benmccann mentioned this pull request Jun 7, 2021
Merged
5 tasks
@Conduitry
Copy link
Member Author

I'm not sure whether this is something that we'd expect esbuild to be able to do, but currently with this PR if you import 'foo' where foo is a CommonJS external dependency which has require('util') (or some other Node builtin), then this essentially remains as a require('util') in the output bundle, and is not transformed into an import 'util'.

The CJS helper that esbuild is using and injecting is:

var __require = (x) => {
  if (typeof require !== "undefined")
    return require(x);
  throw new Error('Dynamic require of "' + x + '" is not supported');
};

(and then later on the bundle has __require('util')).

I'm not sure what can be done to make this work other than to have a global.require = createRequire(import.meta.url) at the top of the bundle.

@Conduitry
add require() shim for requires not transformed by esbuild
015375d 
esbuild doesn't seem to transform require()s of Node builtins into
imports, so define a global.require for them.
@Conduitry
Copy link
Member Author

I've just push a require() shim of the sort I mentioned above. It's an ugly solution, but I don't know what else we can do without needing changes in esbuild itself. I think this issue is serious enough that we shouldn't wait for upstream changes to more nicely support this, although it would still be a good idea to look through esbuild's open issues and see whether this is on their roadmap.

@bleucitron
Copy link
Contributor

Will this also fix #1428 ?

@Conduitry
Copy link
Member Author

It should. That looks like a duplicate of #1257.

@Conduitry Conduitry mentioned this pull request Jun 8, 2021
Closed
@Conduitry Conduitry merged commit 0b780a6 into sveltejs:master Jun 10, 2021
@Conduitry Conduitry deleted the gh-1257 branch June 10, 2021 13:12
@Conduitry Conduitry mentioned this pull request Jun 22, 2021
Closed
@benmccann benmccann mentioned this pull request Jul 3, 2021
Open
@ignatiusmb ignatiusmb mentioned this pull request Aug 9, 2021
Closed
@benmccann benmccann mentioned this pull request Sep 13, 2021
Closed
@Conduitry Conduitry mentioned this pull request Sep 14, 2021
Closed
@benmccann benmccann mentioned this pull request Oct 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rich-Harris Rich-Harris Rich-Harris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@sveltejs/kit' on production build
3 participants
@Conduitry @bleucitron @Rich-Harris